Disable pnpm's implicit install-on-run in favor of explicit installs - #5804
Disable pnpm's implicit install-on-run in favor of explicit installs#5804lukemelia wants to merge 2 commits into
Conversation
Preview deploymentsHost Test Results 1 files ± 0 1 suites ±0 2h 1m 39s ⏱️ + 1h 56m 6s Results for commit 3691ab4. ± Comparison against earlier commit d5e8aba. Realm Server Test Results 1 files ± 0 1 suites ±0 16m 8s ⏱️ + 2m 31s Results for commit 3691ab4. ± Comparison against earlier commit d5e8aba. |
…flake pnpm 11 defaults verify-deps-before-run to `install`, so every `pnpm run` / `pnpm exec` first checks node_modules freshness and silently runs a workspace-wide install when the check fails. In CI the check never passes, even immediately after the init action's `pnpm install --frozen-lockfile` (and even after one of these implicit installs completes), so every pnpm invocation in a job pays a redundant 4-16s install. The fatal case is the service boot: `run-p` launches ~9 pnpm scripts at once, each kicking off its own concurrent install into the same node_modules. They stomp each other's bin links (the "Failed to create bin ... ENOENT" warnings visible in green shards) and occasionally deadlock; when start:matrix's install is the one that wedges, the Synapse container is never created and the shard fails with "Failed to reach Synapse ... after 60 attempts (~300s)". This is the mechanism behind the recurring "Synapse startup race" flake, which hit 12 host shards across three PRs today alone. Setting verifyDepsBeforeRun: false restores pnpm 10 behavior: scripts run against node_modules as-is, and installs happen only when explicitly requested. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
pnpm 11 reads workspace settings (catalog, overrides, patchedDependencies, allowBuilds, verifyDepsBeforeRun) from pnpm-workspace.yaml, but no workflow path filter included it — a settings-only change that doesn't touch the lockfile ran zero suites. Add it alongside pnpm-lock.yaml in every filter, including ci-host's index-cache invalidation case, since overrides and patches can change indexing behavior the same way a lockfile change can. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
d5e8aba to
3691ab4
Compare
richardhjtan
left a comment
richardhjtan
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] I reviewed this as a configuration change with two independent halves: does the setting actually take effect (a mistyped key in pnpm-workspace.yaml fails silently), and is the path-filter change complete (a half-applied filter is worse than none, because it looks handled). I verified both against the checked-out branch rather than reading them.
Bottom line: both halves are correct and complete, and the reasoning in the description holds up. No blocking issues. One follow-up — the change fixes a symptom whose cause is still unexplained, and the artifact that would explain it is one cat away in CI.
The setting takes effect. pnpm config get verify-deps-before-run reports undefined on main and false on this branch, under the pinned pnpm 11.0.9. The key is top-level rather than nested under the catalog: map it follows — the mistake this file's shape invites.
The path-filter change is complete. I enumerated every reference to pnpm-lock.yaml under .github/ and checked each against the diff. Six sites carry it as a change-detection input — the paths: filters in ci-host.yaml, ci-software-factory.yaml, pr-boxel-ui.yml, preview-host.yml, the dorny/paths-filter block in ci.yaml, and the index-cache invalidation case in ci-host.yaml — and all six gained pnpm-workspace.yaml. None was missed.
The seventh reference is correctly left alone, which is worth recording because it looks like a miss. .github/actions/init/action.yml keys the actions/cache step on hashFiles('**/pnpm-lock.yaml'), and that cache's path is pnpm store path — the content-addressed package store, not node_modules. Settings like verifyDepsBeforeRun don't change which tarballs are needed, so adding the file there would only cost cache hits; and the resolution-affecting settings that do change them (catalog, overrides, patchedDependencies) cannot change without moving the lockfile, which already moves the key. Leaving it out is right in both directions.
Also worth noting the index-cache case is the right call, not just a consistent one. pnpm-workspace.yaml carries the catalog, so a version bump there changes what the indexer actually runs — which is the same reason packages/runtime-common/* is in that list. A settings-only edit will now invalidate the index cache unnecessarily, but erring that way costs a cache miss rather than a wrong green.
Follow-up: capture why the freshness check fails, before the comment in pnpm-workspace.yaml becomes the last word on it. Detail and the one-command diagnostic are in the thread on that block.
Generated by Claude Code
| # pnpm 11 defaults this to `install`, making every `pnpm run` first check | ||
| # node_modules freshness and silently run a full install when the check | ||
| # fails. The check never passes in CI, so each of the ~9 services `run-p` | ||
| # boots kicks off its own concurrent workspace-wide install into the same | ||
| # node_modules; when they collide badly enough, start:matrix never reaches | ||
| # `docker run` and the shard dies with "Failed to reach Synapse". Disabling | ||
| # restores pnpm 10 behavior: scripts run against node_modules as-is. | ||
| verifyDepsBeforeRun: false |
There was a problem hiding this comment.
[Claude Code 🤖] Confirmation, plus the one question this change leaves open.
The setting takes effect — verified rather than assumed, since a mistyped or wrongly-placed key in pnpm-workspace.yaml fails silently:
pnpm --version → 11.0.9
main: pnpm config get verify-deps-before-run → undefined
this branch: pnpm config get verify-deps-before-run → false
Placement is right too: the block is top-level, not nested under the catalog: map it follows, which is exactly the mistake this file's shape invites.
false is the correct value here, and it's worth saying why in the comment. pnpm also offers warn, error, and prompt, and warn is the obvious-looking compromise — keep the staleness signal, skip the install. It isn't one, given the premise of this change: if the check never passes in CI, warn emits a warning on every pnpm invocation in every job, and locally on every pnpm <script>. That's noise with no action attached. One sentence saying the middle values were considered and rejected for that reason would stop the next person from "improving" this to warn and then reverting it a week later.
The open question: why does the check fail? The description establishes that it does — including immediately after a clean pnpm install --frozen-lockfile — and that is enough to justify the change. But nothing here says why, and that matters for two reasons: if the divergence is real rather than a pnpm bug, this hides it; and if it is a pnpm bug, it is worth reporting upstream rather than carrying a workspace-local workaround indefinitely.
One command answers it. The check's inputs are recorded in node_modules/.pnpm-workspace-state-v1.json. On this workspace it holds:
lastValidatedTimestamp, projects (per-project manifests, keyed by absolute path),
pnpmfiles, settings, filteredInstall
Dumping that file in a CI job immediately after the init action's install, and again before the first pnpm run, shows which input moved. Two of the fields are already interesting here without any speculation about the cause:
pnpmfileslists this repo's.pnpmfile.cjs. A pnpmfile is a resolution input, so it is one of the things the check compares.filteredInstallisfalse, which matches CI —.github/actions/init/action.ymlruns a single unfilteredpnpm install --frozen-lockfile, and it is the onlypnpm installin.github/— so a filtered/unfiltered mismatch is already ruled out.
Scope. Follow-up, not a blocker. The change stands on its measured effect; this is about not losing the thread on the cause, since the comment above will be read as the final word on it.
2 participants
What
verifyDepsBeforeRun: falseinpnpm-workspace.yaml. pnpm 11 defaults this setting toinstall, which makes everypnpm run/pnpm execcheck node_modules freshness first and silently run a workspace-wide install when the check fails. Withfalse, scripts run against node_modules as-is and installs happen only when explicitly requested — the pnpm 10 behavior this workspace ran on previously.pnpm-workspace.yamlto every workflow path filter that already listspnpm-lock.yaml(and to ci-host's index-cache invalidation case). The file carries dependency-resolution settings (catalog, overrides, patchedDependencies, allowBuilds, verifyDepsBeforeRun), but a settings-only change that doesn't touch the lockfile previously ran zero suites.Why
The freshness check never passes in CI — not even immediately after the init action's
pnpm install --frozen-lockfile, and not after one of the implicit installs itself completes — so every pnpm invocation in a CI job pays a redundant 4–16s install. Green host-shard logs show the sequence plainly: the explicit install finishes, then three more implicit installs run during setup, then several concurrent ones fire whenrun-pboots the test services.That concurrency is the fatal case.
start:matrix,start:smtp,start:host-dist, and the rest are eachpnpm runinvocations, so each spawns its own workspace-wide install into the same node_modules. They stomp each other's bin links (theFailed to create bin … ENOENTwarnings visible even in passing shards) and occasionally wedge each other. Whenstart:matrix's install is the one that hangs,start-matrix.shnever reachespnpm assert-synapse-running, the Synapse container is never created, and the shard fails withFailed to reach Synapse at http://localhost:8008/_matrix/client/versions after 60 attempts (~300s)— the recurring "Synapse startup" flake. Failing-shard logs show that install still running at teardown, killed by SIGTERM five minutes after it started, withError response from daemon: No such container: boxel-synapse-ciin between.The same mechanism slows local dev: every
pnpm <script>pays the check, andpnpm start:allfans out the same concurrent implicit installs.Test plan
pnpm config get verify-deps-before-runreportsfalsewith this change (previously unset, defaulting toinstall).Done in Ns using pnpmimplicit-install output between the init action's install and the test run, and noFailed to create bin … ENOENTwarnings during service boot.🤖 Generated with Claude Code